fix: validate and quote instance name in deploy shell commands#540
Closed
brianwtaylor wants to merge 2 commits intoNVIDIA:mainfrom
Closed
fix: validate and quote instance name in deploy shell commands#540brianwtaylor wants to merge 2 commits intoNVIDIA:mainfrom
brianwtaylor wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughTrim and validate instanceName to an RFC‑1123 label (lowercase, letters/numbers/hyphen, max 63); replace unquoted shell usages with a shell-quoted Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (nemoclaw)
participant Local as Local FS / Shell
participant Remote as Remote VM (instance)
participant Brev as brev binary
CLI->>Local: trim & validate instanceName
Local->>Brev: execFileSync('which', ['brev'])
Brev-->>Local: path / error
Local->>Brev: execFileSync('brev', ['create', qname, ...])
Brev-->>Remote: create instance
Local->>Local: mkdtempSync -> write .env (quoted secrets)
Local->>Remote: scp "qname":/home/.../ .env
Local->>Remote: ssh "qname" './brev-setup.sh' (uses qname)
Remote-->>Local: setup output
Local->>Local: cleanup temp dir/file (finally)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/nemoclaw.js (1)
162-169:⚠️ Potential issue | 🟠 MajorQuote the uploaded env values and include
NGC_API_KEYfallback.The
.envwritten here is sourced again on Line 177, Line 181, and Line 187. RawKEY=valuelines will re-interpret$, backticks,#, or whitespace inside tokens, and deployments where onlyNGC_API_KEYis set still won't send any NGC credential to the remote setup. Serialize each value withshellQuote(...)and addNGC_API_KEY=${process.env.NGC_API_KEY || process.env.NVIDIA_API_KEY}.Suggested fix
- const envLines = [`NVIDIA_API_KEY=${process.env.NVIDIA_API_KEY}`]; + const ngcApiKey = process.env.NGC_API_KEY || process.env.NVIDIA_API_KEY; + const envLines = []; + if (process.env.NVIDIA_API_KEY) { + envLines.push(`NVIDIA_API_KEY=${shellQuote(process.env.NVIDIA_API_KEY)}`); + } + if (ngcApiKey) { + envLines.push(`NGC_API_KEY=${shellQuote(ngcApiKey)}`); + } const ghToken = process.env.GITHUB_TOKEN; - if (ghToken) envLines.push(`GITHUB_TOKEN=${ghToken}`); + if (ghToken) envLines.push(`GITHUB_TOKEN=${shellQuote(ghToken)}`); const tgToken = getCredential("TELEGRAM_BOT_TOKEN"); - if (tgToken) envLines.push(`TELEGRAM_BOT_TOKEN=${tgToken}`); + if (tgToken) envLines.push(`TELEGRAM_BOT_TOKEN=${shellQuote(tgToken)}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/nemoclaw.js` around lines 162 - 169, The .env file is written without shell-quoting and misses the NGC fallback; update the envLines construction (where envLines is created and pushed to) to serialize each value with a shell quoting helper (e.g., shellQuote(value)) instead of raw interpolation, and add an entry for NGC_API_KEY using the fallback expression process.env.NGC_API_KEY || process.env.NVIDIA_API_KEY; ensure the TELEGRAM_BOT_TOKEN and GITHUB_TOKEN pushes (from getCredential("TELEGRAM_BOT_TOKEN") and process.env.GITHUB_TOKEN) also use shellQuote, then keep writing envTmp and calling runScp as before.
🧹 Nitpick comments (1)
bin/nemoclaw.js (1)
20-34: DeduplicateshellQuotebefore the two copies drift.This file imports a shared quoting helper on Line 20 but still keeps a second implementation locally. For security-sensitive escaping, one source of truth is safer.
Suggested cleanup
-const { validateInstanceName, runSsh, runScp, runRsync, shellQuote: deployShellQuote } = require("./lib/deploy"); +const { validateInstanceName, runSsh, runScp, runRsync, shellQuote } = require("./lib/deploy"); ... -function shellQuote(value) { - return `'${String(value).replace(/'/g, `'\\''`)}'`; -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/nemoclaw.js` around lines 20 - 34, The file defines a local shellQuote function that duplicates the imported shellQuote (aliased as deployShellQuote) from ./lib/deploy; remove the local shellQuote implementation and replace its usages with the imported deployShellQuote (or rename the import to shellQuote if you prefer) so there is a single source of truth for quoting; update any references in this file to use the imported symbol (validateInstanceName, runSsh, runScp, runRsync remain unchanged) and ensure module.exports/usage remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/runner.js`:
- Around line 100-117: runCaptureArgv currently calls sanitizeEnv(env) inside
the try block so a blocked env override can be swallowed by opts.ignoreError;
pull the env validation/normalization out of the try so sanitizeEnv is called
before executing the child process and before the catch that honors
opts.ignoreError, then pass the resulting sanitizedEnv (or merged env object)
into execFileSync; update references in runCaptureArgv to use sanitizedEnv and
keep the rest of the try/catch behavior unchanged.
In `@bin/nemoclaw.js`:
- Around line 167-170: The current temp file creation using envTmp =
path.join(os.tmpdir(), `nemoclaw-env-${Date.now()}`) is predictable and unsafe;
replace that with a secure temp directory created by fs.mkdtempSync (e.g. base =
fs.mkdtempSync(path.join(os.tmpdir(), 'nemoclaw-'))) and then create the secrets
file inside that dir using fs.writeFileSync with the flag: "wx" to avoid
following symlinks and clobbering; ensure the write, call to runScp(envTmp, ...)
and fs.unlinkSync cleanup are all wrapped in a try/finally so the temp file (and
temp dir) are removed even on error.
---
Outside diff comments:
In `@bin/nemoclaw.js`:
- Around line 162-169: The .env file is written without shell-quoting and misses
the NGC fallback; update the envLines construction (where envLines is created
and pushed to) to serialize each value with a shell quoting helper (e.g.,
shellQuote(value)) instead of raw interpolation, and add an entry for
NGC_API_KEY using the fallback expression process.env.NGC_API_KEY ||
process.env.NVIDIA_API_KEY; ensure the TELEGRAM_BOT_TOKEN and GITHUB_TOKEN
pushes (from getCredential("TELEGRAM_BOT_TOKEN") and process.env.GITHUB_TOKEN)
also use shellQuote, then keep writing envTmp and calling runScp as before.
---
Nitpick comments:
In `@bin/nemoclaw.js`:
- Around line 20-34: The file defines a local shellQuote function that
duplicates the imported shellQuote (aliased as deployShellQuote) from
./lib/deploy; remove the local shellQuote implementation and replace its usages
with the imported deployShellQuote (or rename the import to shellQuote if you
prefer) so there is a single source of truth for quoting; update any references
in this file to use the imported symbol (validateInstanceName, runSsh, runScp,
runRsync remain unchanged) and ensure module.exports/usage remains consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23ecbbc7-22d0-4a30-b216-d0762e89c1e1
📒 Files selected for processing (4)
bin/lib/deploy.jsbin/lib/runner.jsbin/nemoclaw.jstest/deploy.test.js
brianwtaylor
force-pushed
the
security/shell-injection-ngc-key
branch
from
30cf038 to
0e26d16
Compare
brianwtaylor
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/deploy-instance-name.test.js (3)
33-35: Function extraction regex is format-dependent and may break on refactoring.The non-greedy pattern
/async function deploy\([\s\S]*?\n\}/assumes the function's closing brace appears at column 0 (directly after newline). If the code is reformatted or a nested object/function uses the same pattern, this extraction could capture an incomplete body.Consider extracting with a parser (e.g.,
acorn) or using a more robust boundary like matching balanced braces, though for a sanity-check test, this is acceptable if the limitation is understood.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deploy-instance-name.test.js` around lines 33 - 35, The current extraction using source.match(/async function deploy\([\s\S]*?\n\}/) (deployMatch/deployBody) is brittle; replace it by parsing the file and locating the deploy function node (e.g., use acorn.parse/sourceType: "module" and find a FunctionDeclaration named "deploy") or, if you want a minimal change, locate the index of "async function deploy(" and scan forward counting braces to capture the balanced function body so nested braces or reformatting won't break the test.
16-28: Weak substring checks could pass with unrelated code.The assertions only check for substring presence (
[a-z0-9-]and253), which could be satisfied by unrelated code. These serve as basic regression guards but won't catch validation being removed if similar strings exist elsewhere.Acceptable for a sanity test, but consider checking for the full regex pattern or the specific condition line for stronger guarantees.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deploy-instance-name.test.js` around lines 16 - 28, The current tests in test/deploy-instance-name.test.js use weak substring checks against the file string (variable source) which can be falsely satisfied; update the assertions to verify the exact validation expression or condition used by deploy() instead of just substrings—e.g., assert that source contains the full RFC1123 regex pattern string (the full bracketed class plus anchors and quantifier) or the exact line that enforces length 253 (the deploy() validation/if statement), so the test fails if the actual validation code is removed or altered; locate checks referencing source and deploy() and replace the two assert.ok calls with assertions that match the full regex/condition text.
42-46: Edge case: detection regex may miss boundary positions.The regex
/[^"]\$\{name\}[^"]/requires a non-quote character on both sides of${name}. If${name}appears at the very start or end of a template string without padding, it won't be detected:`${name}` // backtick before $, but nothing after } before backtickThe current code doesn't have such cases, so this works in practice. A stricter negative lookahead/lookbehind pattern could improve robustness if needed:
const unquoted = line.match(/(?<!")\$\{name\}(?!")/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deploy-instance-name.test.js` around lines 42 - 46, The current detection for unquoted ${name} in the test uses the regex /[^"]\$\{name\}[^"]/ assigned to the variable unquoted, which misses cases when ${name} is at the start or end of the line; update the match logic in test/deploy-instance-name.test.js (the unquoted variable) to use boundary-aware negative lookbehind/lookahead (e.g., ensure ${name} is not immediately preceded or followed by a quote) so occurrences at string boundaries are detected; if your environment may not support lookbehind, implement equivalent checks by testing the character before/after the match or using a regex that anchors to line boundaries as a fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/deploy-instance-name.test.js`:
- Around line 33-35: The current extraction using source.match(/async function
deploy\([\s\S]*?\n\}/) (deployMatch/deployBody) is brittle; replace it by
parsing the file and locating the deploy function node (e.g., use
acorn.parse/sourceType: "module" and find a FunctionDeclaration named "deploy")
or, if you want a minimal change, locate the index of "async function deploy("
and scan forward counting braces to capture the balanced function body so nested
braces or reformatting won't break the test.
- Around line 16-28: The current tests in test/deploy-instance-name.test.js use
weak substring checks against the file string (variable source) which can be
falsely satisfied; update the assertions to verify the exact validation
expression or condition used by deploy() instead of just substrings—e.g., assert
that source contains the full RFC1123 regex pattern string (the full bracketed
class plus anchors and quantifier) or the exact line that enforces length 253
(the deploy() validation/if statement), so the test fails if the actual
validation code is removed or altered; locate checks referencing source and
deploy() and replace the two assert.ok calls with assertions that match the full
regex/condition text.
- Around line 42-46: The current detection for unquoted ${name} in the test uses
the regex /[^"]\$\{name\}[^"]/ assigned to the variable unquoted, which misses
cases when ${name} is at the start or end of the line; update the match logic in
test/deploy-instance-name.test.js (the unquoted variable) to use boundary-aware
negative lookbehind/lookahead (e.g., ensure ${name} is not immediately preceded
or followed by a quote) so occurrences at string boundaries are detected; if
your environment may not support lookbehind, implement equivalent checks by
testing the character before/after the match or using a regex that anchors to
line boundaries as a fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 701d46dd-8d50-4eb8-a021-3a3d64c6c2b2
📒 Files selected for processing (2)
bin/nemoclaw.jstest/deploy-instance-name.test.js
brianwtaylor
force-pushed
the
security/shell-injection-ngc-key
branch
from
0e26d16 to
e3ccf7c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/deploy-instance-name.test.js`:
- Around line 33-35: The regex used to extract the deploy function (deployMatch
via /async function deploy\([\s\S]*?\n\}/) is non‑greedy and may stop at the
first inner "}" — replace the fragile regex approach with a brace‑depth
extraction: locate the "async function deploy(" start, then iterate characters
from the first "{" tracking open/close brace depth until depth returns to zero
and set deployBody to that full slice; update references to
deployMatch/deployBody accordingly so the entire function is captured reliably.
- Around line 40-46: The current detection uses unquoted =
line.match(/[^"]\$\{name\}[^"]/) which misses `${name}` at string boundaries;
instead iterate each occurrence of `${name}` in variable line (e.g., using
matchAll on /\$\{name\}/) and assert that the character immediately before and
after the match are double quotes so every occurrence is exactly `"${name}"`;
update the check around the existing if (line.includes("${name}") &&
!line.match(/console\.(log|error)/)) and replace the unquoted/assert.ok logic
(references: variable line, unquoted, and the assert.ok call) accordingly.
- Around line 16-28: The current checks only assert that tokens exist in the
file via source.includes and can pass on unrelated text; replace these loose
assertions with behavioral and/or structural checks: import or require the
deploy function and add tests that call deploy() with representative valid and
invalid instance names (e.g., valid "abc-123", invalid uppercase or illegal
chars, and a >253-character string) and assert acceptance/rejection, and/or
parse the source AST to assert the deploy function body contains a RFC1123 regex
literal and a numeric 253 length check; update the tests referencing source and
deploy() to validate actual behavior rather than mere token presence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8efbbf8-a9ff-4c06-8004-b311d211ab52
📒 Files selected for processing (2)
bin/nemoclaw.jstest/deploy-instance-name.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/nemoclaw.js
Extends the RFC 1123 validation and double-quoting pattern from NVIDIA#170 (sandbox names) to the deploy() function. All 7 unquoted ${name} interpolations in ssh/scp/rsync commands are now quoted, and instance names are validated at entry to prevent command injection.
brianwtaylor
force-pushed
the
security/shell-injection-ngc-key
branch
from
e3ccf7c to
a1b2e96
Compare
ericksoa
added a commit
that referenced
this pull request
Mar 21, 2026
Comprehensive fix for shell injection vulnerabilities where user input (instance names, sandbox names, model names, API keys) was interpolated unsanitized into shell commands via run()/runInteractive()/execSync(). Changes: - Add shellQuote() and validateName() to runner.js as shared utilities - Replace all execSync() with execFileSync() in deploy (no shell) - Apply shellQuote() to every user-controlled variable in shell commands across nemoclaw.js, onboard.js, nim.js, policies.js - Add RFC 1123 name validation at CLI dispatch for sandbox/instance names - Fix path traversal in policies.js loadPreset() - Replace predictable temp file with mkdtempSync() - Remove duplicate shellQuote() definitions (now single source in runner.js) - 9 new test cases for shellQuote, validateName, and path traversal Supersedes #55, #110, #475, #540, #48, #171.
Contributor
Author
|
Closing in favor of #584 — ericksoa's comprehensive command injection fix covers this and more (centralized shellQuote, validateName, execFileSync migration across all entry points). |
Align with ericksoa's NVIDIA#584 approach: - Centralize shellQuote in runner.js, remove local copy from nemoclaw.js - Replace double-quoting with shellQuote() single-quote wrapping - Switch execSync to execFileSync in deploy() - Quote env values (API keys, tokens) with shellQuote - Max 63 chars (RFC 1123 label) instead of 253 (subdomain) - Update tests to verify shellQuote patterns
Contributor
Author
|
Closing in favor of #584. Updated branch to align with ericksoa's centralized shellQuote pattern for reference. |
kjw3
pushed a commit
that referenced
this pull request
Mar 21, 2026
* security: fix command injection across all CLI entry points Comprehensive fix for shell injection vulnerabilities where user input (instance names, sandbox names, model names, API keys) was interpolated unsanitized into shell commands via run()/runInteractive()/execSync(). Changes: - Add shellQuote() and validateName() to runner.js as shared utilities - Replace all execSync() with execFileSync() in deploy (no shell) - Apply shellQuote() to every user-controlled variable in shell commands across nemoclaw.js, onboard.js, nim.js, policies.js - Add RFC 1123 name validation at CLI dispatch for sandbox/instance names - Fix path traversal in policies.js loadPreset() - Replace predictable temp file with mkdtempSync() - Remove duplicate shellQuote() definitions (now single source in runner.js) - 9 new test cases for shellQuote, validateName, and path traversal Supersedes #55, #110, #475, #540, #48, #171. * fix: deduplicate shellQuote in local-inference.js Import shellQuote from runner.js instead of defining a local copy. Single source of truth for shell quoting across the codebase. * security: fix telegram bridge injection + add regression guards Telegram bridge: - Replace execSync with execFileSync for ssh-config retrieval - shellQuote message, API key, and session ID in remote command - Validate SANDBOX_NAME at startup - Use mkdtempSync for temp SSH config (not predictable path) Regression tests: - nemoclaw.js must not use execSync - Single shellQuote definition in bin/ - CLI rejects malicious sandbox names (e2e, no mocking) - telegram-bridge.js validates SANDBOX_NAME and avoids execSync * security: address CodeRabbit review findings on shell injection PR - Shell-quote secret values written to .env before remote source - Wrap scp upload in try/finally to guarantee temp secret cleanup - Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard - Replace predictable Date.now() temp path with mkdtempSync in policies - Strengthen e2e test with canary file to prove payload never executes - Fix merge-introduced test expectations for shellQuote single-quote format
jnun
pushed a commit
to jnun/EasyClaw
that referenced
this pull request
Mar 21, 2026
) * security: fix command injection across all CLI entry points Comprehensive fix for shell injection vulnerabilities where user input (instance names, sandbox names, model names, API keys) was interpolated unsanitized into shell commands via run()/runInteractive()/execSync(). Changes: - Add shellQuote() and validateName() to runner.js as shared utilities - Replace all execSync() with execFileSync() in deploy (no shell) - Apply shellQuote() to every user-controlled variable in shell commands across nemoclaw.js, onboard.js, nim.js, policies.js - Add RFC 1123 name validation at CLI dispatch for sandbox/instance names - Fix path traversal in policies.js loadPreset() - Replace predictable temp file with mkdtempSync() - Remove duplicate shellQuote() definitions (now single source in runner.js) - 9 new test cases for shellQuote, validateName, and path traversal Supersedes NVIDIA#55, NVIDIA#110, NVIDIA#475, NVIDIA#540, NVIDIA#48, NVIDIA#171. * fix: deduplicate shellQuote in local-inference.js Import shellQuote from runner.js instead of defining a local copy. Single source of truth for shell quoting across the codebase. * security: fix telegram bridge injection + add regression guards Telegram bridge: - Replace execSync with execFileSync for ssh-config retrieval - shellQuote message, API key, and session ID in remote command - Validate SANDBOX_NAME at startup - Use mkdtempSync for temp SSH config (not predictable path) Regression tests: - nemoclaw.js must not use execSync - Single shellQuote definition in bin/ - CLI rejects malicious sandbox names (e2e, no mocking) - telegram-bridge.js validates SANDBOX_NAME and avoids execSync * security: address CodeRabbit review findings on shell injection PR - Shell-quote secret values written to .env before remote source - Wrap scp upload in try/finally to guarantee temp secret cleanup - Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard - Replace predictable Date.now() temp path with mkdtempSync in policies - Strengthen e2e test with canary file to prove payload never executes - Fix merge-introduced test expectations for shellQuote single-quote format
Ryuketsukami
pushed a commit
to Ryuketsukami/NemoClaw
that referenced
this pull request
Mar 24, 2026
) * security: fix command injection across all CLI entry points Comprehensive fix for shell injection vulnerabilities where user input (instance names, sandbox names, model names, API keys) was interpolated unsanitized into shell commands via run()/runInteractive()/execSync(). Changes: - Add shellQuote() and validateName() to runner.js as shared utilities - Replace all execSync() with execFileSync() in deploy (no shell) - Apply shellQuote() to every user-controlled variable in shell commands across nemoclaw.js, onboard.js, nim.js, policies.js - Add RFC 1123 name validation at CLI dispatch for sandbox/instance names - Fix path traversal in policies.js loadPreset() - Replace predictable temp file with mkdtempSync() - Remove duplicate shellQuote() definitions (now single source in runner.js) - 9 new test cases for shellQuote, validateName, and path traversal Supersedes NVIDIA#55, NVIDIA#110, NVIDIA#475, NVIDIA#540, NVIDIA#48, NVIDIA#171. * fix: deduplicate shellQuote in local-inference.js Import shellQuote from runner.js instead of defining a local copy. Single source of truth for shell quoting across the codebase. * security: fix telegram bridge injection + add regression guards Telegram bridge: - Replace execSync with execFileSync for ssh-config retrieval - shellQuote message, API key, and session ID in remote command - Validate SANDBOX_NAME at startup - Use mkdtempSync for temp SSH config (not predictable path) Regression tests: - nemoclaw.js must not use execSync - Single shellQuote definition in bin/ - CLI rejects malicious sandbox names (e2e, no mocking) - telegram-bridge.js validates SANDBOX_NAME and avoids execSync * security: address CodeRabbit review findings on shell injection PR - Shell-quote secret values written to .env before remote source - Wrap scp upload in try/finally to guarantee temp secret cleanup - Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard - Replace predictable Date.now() temp path with mkdtempSync in policies - Strengthen e2e test with canary file to prove payload never executes - Fix merge-introduced test expectations for shellQuote single-quote format
jessesanford
pushed a commit
to jessesanford/NemoClaw
that referenced
this pull request
Mar 24, 2026
) * security: fix command injection across all CLI entry points Comprehensive fix for shell injection vulnerabilities where user input (instance names, sandbox names, model names, API keys) was interpolated unsanitized into shell commands via run()/runInteractive()/execSync(). Changes: - Add shellQuote() and validateName() to runner.js as shared utilities - Replace all execSync() with execFileSync() in deploy (no shell) - Apply shellQuote() to every user-controlled variable in shell commands across nemoclaw.js, onboard.js, nim.js, policies.js - Add RFC 1123 name validation at CLI dispatch for sandbox/instance names - Fix path traversal in policies.js loadPreset() - Replace predictable temp file with mkdtempSync() - Remove duplicate shellQuote() definitions (now single source in runner.js) - 9 new test cases for shellQuote, validateName, and path traversal Supersedes NVIDIA#55, NVIDIA#110, NVIDIA#475, NVIDIA#540, NVIDIA#48, NVIDIA#171. * fix: deduplicate shellQuote in local-inference.js Import shellQuote from runner.js instead of defining a local copy. Single source of truth for shell quoting across the codebase. * security: fix telegram bridge injection + add regression guards Telegram bridge: - Replace execSync with execFileSync for ssh-config retrieval - shellQuote message, API key, and session ID in remote command - Validate SANDBOX_NAME at startup - Use mkdtempSync for temp SSH config (not predictable path) Regression tests: - nemoclaw.js must not use execSync - Single shellQuote definition in bin/ - CLI rejects malicious sandbox names (e2e, no mocking) - telegram-bridge.js validates SANDBOX_NAME and avoids execSync * security: address CodeRabbit review findings on shell injection PR - Shell-quote secret values written to .env before remote source - Wrap scp upload in try/finally to guarantee temp secret cleanup - Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard - Replace predictable Date.now() temp path with mkdtempSync in policies - Strengthen e2e test with canary file to prove payload never executes - Fix merge-introduced test expectations for shellQuote single-quote format
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Continuation of the hardening started in #170 — extends RFC 1123 validation and double-quoting to the
deploy()function.Summary
deploy()(same regex as sandbox names from fix: validate and quote sandbox name in shell commands #170)${name}interpolations in ssh, scp, and rsync shell commandsnemoclaw-env-${Date.now()}temp path withfs.mkdtempSync()+ exclusive create (flag: "wx") to prevent symlink race on shared machinessetup-sandbox-name.test.js)Motivation
The
deploy()function interpolates the user-supplied instance name into shell commands (ssh,scp,rsync,brev create) without quoting. A crafted instance name could inject arbitrary shell commands. Additionally, the secrets temp file used a predictable path vulnerable to symlink races.Test plan
Automated Tests
All 6 new tests pass. 177/177 total tests run; 3 pre-existing failures in
installer runtime preflight(unrelated, present on main).Summary by CodeRabbit
Bug Fixes
Tests